fix(workspace-update): prompt for option drift and immutable params by EhabY · Pull Request #967 · coder/vscode-coder

EhabY · 2026-05-20T10:19:09Z

Aligns workspace-update parameter collection with the dashboard's getMissingParameters (coder/site/src/api/api.ts:39). Two cases previously slipped through and produced server-side build rejections with no UI:

A stored option/multi-select value that is no longer one of the new template version's options (option-value drift) was carried forward verbatim.
An immutable parameter without a stored value was never prompted, even though it can never be set after the first build.

needsPrompt(param, storedValue) centralizes the decision; multi-select drift is also detected (parsing the JSON-encoded stored value) because the dashboard explicitly skips multi-select.

On a multi-select drift re-prompt the QuickPick now pre-selects the picks that are still valid, so the user does not have to re-select what was already correct. Non-required multi-selects can now also submit zero picks (previously the prompt stayed open with no way to express "none").

Behavior matrix

Case	Behavior	Vs. dashboard
No stored value, mutable + required, no default	Prompt	match
No stored value, mutable + required, has default	Skip	deviate (server applies default; avoid auto-update friction)
No stored value, immutable	Prompt	match
No stored value, not required (mutable)	Skip	match
Stored value still in new options	Skip	match
Single-select drift	Prompt (no preselect)	match
Multi-select drift	Prompt with valid picks pre-selected	beyond (dashboard skips multi-select)
Multi-select stored value not valid JSON	Prompt	beyond (defensive)
Non-required multi-select submitted empty	Accept as `"[]"`	n/a (pre-existing prompt code)

Files

src/api/updateParameters.ts: needsPrompt, parseMultiSelectValue; promptForParameter accepts storedValue and pre-selects still-valid multi-select picks.
test/unit/api/updateParameters.test.ts: 12 new cases covering single/multi drift, immutable + default, multi-select happy path, empty-array stored value, empty submission, and preselect verification.
CHANGELOG.md: user-facing entry under Unreleased > Fixed.

EhabY · 2026-05-20T10:30:23Z

/coder-agents-review

coder-agents-review

Clean, well-scoped fix. The needsPrompt extraction is solid, the behavior table in the PR description is exactly the kind of documentation that saves future debugging time, and 6 of 7 new tests fail on the base branch (Netero verified). The deviation comment at line 55 is a model for documenting intentional divergence.

One design concern (P2), two P3s, one P3 test gap, one P4, and three nits.

The P2 is about the default_value short-circuit applying to immutable parameters. When a new immutable param has a default, the user is never prompted, and because the param is immutable, they can never override it afterward. The dashboard prompts in this case. The PR documents this as intentional ("avoid auto-update friction"), but the friction argument holds for mutable params (temporary inconvenience), not immutable ones (permanent lock-in).

Melody walked all 11 ParameterFormType variants and confirmed the dispatch/filter pairing is clean. Chopper verified edge cases ("", "[]", invalid JSON). Mafu-san traced the alignment claim against the actual dashboard code and confirmed the logic matches where claimed.

"The user chose correctly; the tool failed to preserve what was still good." (Hisoka, on multi-select drift discarding valid picks)

🤖 This review was automatically generated with Coder Agents.

EhabY · 2026-05-20T12:37:38Z

/coder-agents-review

coder-agents-review

All 9 round-1 findings addressed. The P2 (immutable default lock-in) is fixed at the root: if (!param.mutable) return true now runs before the default_value check. Kite, Pariston, Mafuuu, and Chopper independently confirmed the fix is complete. Melody walked 8 pairing modes across the updated code and found zero mismatches. Pariston's adversarial analysis couldn't build a case against the change.

One P3 test gap and two nits remain. The P3 is a missing test for the required-multi-select empty-submission guard; the behavior is correct but unprotected against regression.

"I tried to build a case against this change and couldn't." (Pariston)

🤖 This review was automatically generated with Coder Agents.

Aligns parameter collection with the dashboard's getMissingParameters in coder/site/src/api/api.ts. Previously the filter only checked `required && !default_value` against `name in storedValues`, so: - Required params whose stored option value was no longer in the new version's option list (option-value drift) were carried forward and the build was rejected server-side with no useful UI. - Immutable params without a stored value were never prompted, even though they can never be set after the first build. Add `needsPrompt(param, storedValue)` covering both cases. Beyond the dashboard, also handle multi-select drift by parsing the JSON-encoded value (dashboard explicitly skips multi-select). The `default_value` short-circuit is preserved for the unset case so the server can fall back to the template default during auto-update without a prompt.

- needsPrompt: prompt immutable + default (no user override possible later — permanent lock-in to server default otherwise). Per DEREM-3. - promptForParameter: accept empty selection on non-required multi-select re-prompts; the previous logic kept the QuickPick open with no way to express "none." Per DEREM-5. - promptForParameter: pre-select still-valid picks on multi-select drift re-prompts so the user doesn't lose what was already correct. Per DEREM-7. - Inline notes on both deviations from the dashboard's getMissingParameters (default short-circuit + multi-select drift) so future syncs catch them. - Drop manual `(v): v is string =>` predicate (TS 5.5+ inference). - Tests: multi-select happy path, '[]' empty array, non-required empty submission, preselect verification, immutable + default prompt. - CHANGELOG: user-facing wording ("closing a gap with the web dashboard").

code-asher

I did not get a chance to test, but it looks like an improvement!

Though, thinking about this some more, I think we really do want a --json flag on coder update so we can parse the errors to give us parity with the dashboard.

I think we are going to be constantly chasing after drifting behavior. 😭

We also can never fully validate dynamic parameters client-side...for full parity we would also have to use the dynamic parameters web socket to validate parameters server-side (copying the parameters page basically).

That would be a good amount of work though, maybe this is enough for most cases.

code-asher · 2026-05-21T22:47:56Z

+		// Accepting the default locks it in.
+		if (!param.mutable) return true;


I am not sure I understand the comment; it talks about accepting a default, but the actual code seems to just say non-mutable values need to be prompted.

Oh is it saying that because the default would get locked in permanently, we want to give the user a chance to configure it rather than use the default?

code-asher · 2026-05-21T22:49:06Z

 	return collected;
 }

+/** Based on the dashboard's `getMissingParameters` (coder/site/src/api/api.ts). */


Perhaps important to add a note that the dashboard's version of this is for legacy parameters, not something used for dynamic params, so this is just best-effort (to avoid giving the impression this is what the dashboard normally uses for updates).

code-asher · 2026-05-21T22:50:00Z

+		// Deviation: dashboard prompts mutable+required even when a default
+		// exists; we skip so the server can apply the default during
+		// auto-update without surfacing a modal.
+		return param.required && !param.default_value;


Should this be return param.required? A parameter with a default is never marked as required, which allows blank defaults (at least for dynamic params, not sure about legacy params).

code-asher · 2026-05-21T22:55:19Z

+
+/** Multi-select values are stored as a JSON-encoded string array. */
+function parseMultiSelectValue(raw: string): string[] | null {
+	let parsed: unknown;


nbd but could just put this all in the try and avoid a floating let

code-asher · 2026-05-21T23:14:19Z

 		qp.canSelectMany = multi;
 		qp.ignoreFocusOut = true;
+		if (multi && storedValue !== undefined) {
+			const previous = new Set(parseMultiSelectValue(storedValue) ?? []);


Is the set conversion for performance or something? Could just do .includes instead of .has but idk if there was a reason for the set. nbd though, just curious.

EhabY self-assigned this May 20, 2026

coder-agents-review Bot reviewed May 20, 2026

View reviewed changes

EhabY force-pushed the fix/workspace-update-param-drift branch 2 times, most recently from fa6a8a5 to 2e176c5 Compare May 20, 2026 12:35

coder-agents-review Bot approved these changes May 20, 2026

View reviewed changes

Comment thread test/unit/api/updateParameters.test.ts

Comment thread src/api/updateParameters.ts Outdated

Comment thread src/api/updateParameters.ts Outdated

EhabY force-pushed the fix/workspace-update-param-drift branch from 2e176c5 to bd42d05 Compare May 20, 2026 13:31

EhabY requested a review from code-asher May 20, 2026 15:37

EhabY added 2 commits May 21, 2026 18:14

EhabY force-pushed the fix/workspace-update-param-drift branch from bd42d05 to 3017bbf Compare May 21, 2026 15:14

code-asher approved these changes May 21, 2026

View reviewed changes

		// Accepting the default locks it in.
		if (!param.mutable) return true;

Conversation

EhabY commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Behavior matrix

Files

Uh oh!

EhabY commented May 20, 2026

Uh oh!

coder-agents-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EhabY commented May 20, 2026

Uh oh!

coder-agents-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

code-asher left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

code-asher May 21, 2026

Choose a reason for hiding this comment

Uh oh!

code-asher May 21, 2026

Choose a reason for hiding this comment

Uh oh!

code-asher May 21, 2026

Choose a reason for hiding this comment

Uh oh!

code-asher May 21, 2026

Choose a reason for hiding this comment

Uh oh!

code-asher May 21, 2026

Choose a reason for hiding this comment

Uh oh!

code-asher May 21, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

EhabY commented May 20, 2026 •

edited

Loading

code-asher left a comment •

edited

Loading